Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deduplication mismatches in vtables leading to upcasting unsoundness #135318

Merged
merged 6 commits into from
Jan 31, 2025

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jan 10, 2025

We currently have two cases where subtleties in supertraits can trigger disagreements in the vtable layout, e.g. leading to a different vtable layout being accessed at a callsite compared to what was prepared during unsizing. Namely:

#135315

In this example, we were not normalizing supertraits when preparing vtables. In the example,

trait Supertrait<T> {
    fn _print_numbers(&self, mem: &[usize; 100]) {
        println!("{mem:?}");
    }
}
impl<T> Supertrait<T> for () {}

trait Identity {
    type Selff;
}
impl<Selff> Identity for Selff {
    type Selff = Selff;
}

trait Middle<T>: Supertrait<()> + Supertrait<T> {
    fn say_hello(&self, _: &usize) {
        println!("Hello!");
    }
}
impl<T> Middle<T> for () {}

trait Trait: Middle<<() as Identity>::Selff> {}
impl Trait for () {}

fn main() {
    (&() as &dyn Trait as &dyn Middle<()>).say_hello(&0);
}

When we prepare dyn Trait, we see a supertrait of Middle<<() as Identity>::Selff>, which itself has two supertraits Supertrait<()> and Supertrait<<() as Identity>::Selff>. These two supertraits are identical, but they are not duplicated because we were using structural equality and not considering normalization. This leads to a vtable layout with two trait pointers.

When we upcast to dyn Middle<()>, those two supertraits are now the same, leading to a vtable layout with only one trait pointer. This leads to an offset error, and we call the wrong method.

#135316

This one is a bit more interesting, and is the bulk of the changes in this PR. It's a bit similar, except it uses binder equality instead of normalization to make the compiler get confused about two vtable layouts. In the example,

trait Supertrait<T> {
    fn _print_numbers(&self, mem: &[usize; 100]) {
        println!("{mem:?}");
    }
}
impl<T> Supertrait<T> for () {}

trait Trait<T, U>: Supertrait<T> + Supertrait<U> {
    fn say_hello(&self, _: &usize) {
        println!("Hello!");
    }
}
impl<T, U> Trait<T, U> for () {}

fn main() {
    (&() as &'static dyn for<'a> Trait<&'static (), &'a ()>
        as &'static dyn Trait<&'static (), &'static ()>)
        .say_hello(&0);
}

When we prepare the vtable for dyn for<'a> Trait<&'static (), &'a ()>, we currently consider the PolyTraitRef of the vtable as the key for a supertrait. This leads two two supertraits -- Supertrait<&'static ()> and for<'a> Supertrait<&'a ()>.

However, we can upcast1 without offsetting the vtable from dyn for<'a> Trait<&'static (), &'a ()> to dyn Trait<&'static (), &'static ()>. This is just instantiating the principal trait ref for a specific 'a = 'static. However, when considering those supertraits, we now have only one distinct supertrait -- Supertrait<&'static ()> (which is deduplicated since there are two supertraits with the same substitutions). This leads to similar offsetting issues, leading to the wrong method being called.

The solution here is to recognize that a vtable isn't really meaningfully higher ranked, and to just treat a vtable as corresponding to a TraitRef so we can do this deduplication more faithfully. That is to say, the vtable for dyn for<'a> Tr<'a> and dyn Tr<'x> are always identical, since they both would correspond to a set of free regions on an impl... Do note that Tr<for<'a> fn(&'a ())> and Tr<fn(&'static ())> are still distinct.


There's a bit more that can be cleaned up. In codegen, we can stop using PolyExistentialTraitRef basically everywhere. We can also fix SMIR to stop storing PolyExistentialTraitRef in its vtable allocations.

As for testing, it's difficult to actually turn this into something that can be tested with rustc_dump_vtable, since having multiple supertraits that are identical is a recipe for ambiguity errors. Maybe someone else is more creative with getting that attr to work, since the tests I added being run-pass tests is a bit unsatisfying. Miri also doesn't help here, since it doesn't really generate vtables that are offset by an index in the same way as codegen.

r? @lcnr for the vibe check? Or reassign, idk. Maybe let's talk about whether this makes sense.

(I guess an alternative would also be to not do any deduplication of vtable supertraits (or only a really conservative subset) rather than trying to normalize and deduplicate more faithfully here. Not sure if that works and is sufficient tho.)

cc @steffahn -- ty for the minimizations
cc @WaffleLapkin -- since you're overseeing the feature stabilization :3

Fixes #135315
Fixes #135316

Footnotes

  1. I say upcast but this is a cast that is allowed on stable, since it's not changing the vtable at all, just instantiating the binder of the principal trait ref for some lifetime.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@RalfJung
Copy link
Member

Miri also doesn't help here, since it doesn't really generate vtables that are offset by an index in the same way as codegen.

We do have assertions that are intended to ensure that looking up the method via the offset leads to the same result as the type-driven "direct" lookup. If those did not catch this problem, something seems odd. Note that these are debug assertions so you won't see this in cargo miri or on the playground, but if you run tests via ./x test debug assertions should be on.

@lcnr
Copy link
Contributor

lcnr commented Jan 10, 2025

vibeck 👍 can talk about it regardless, but this feels very reasonable to me modulo the nit above

wrt to hr-trait objects, the important part is that all trait object args are invariant so you can't subtype types in the args: going from dyn Trait<for<'a> fn(&'a ()>> to dyn Trait<fn(&'erased ())> is impossible and would be able to change the vtable. The vtable for higher ranked trait bounds has to be the same as for instantiated ones

@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jan 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@compiler-errors
Copy link
Member Author

I've reworked the rustc_dump_vtable attr to be a bit more eager. You can put it on either:

  1. Non-generic impls, to generate the vtable for that impl.
  2. Non-generic type aliases of dyn Trait, i.e. type X = dyn Trait, to generate the vtable of that object type.

I've also removed the print_vtable_sizes attr, since that's no longer needed. I don't think we're doing any more investigation about vtable sizes these days, and it wasn't working correctly with unnormalized types anyways.

@compiler-errors
Copy link
Member Author

I've also changed a bunch of PolyExistentialTraitRef to ExistentialTraitRef in codegen to reflect the fact that vtables are not higher ranked.

@steffahn
Copy link
Member

I've tested out the new behavior (before the force-push, but it sounds like that wasn't changing anything about it) a little bit, and it seems quite convincing to me :-)

@compiler-errors
Copy link
Member Author

didnt change anything behaviorally since the rebase

@RalfJung
Copy link
Member

RalfJung commented Jan 11, 2025

FWIW running the test from #135315 in Miri with debug assertions does ICE, so the existing check do in fact enable Miri to catch problems like this:

thread 'rustc' panicked at /home/r/src/rust/rustc.2/compiler/rustc_const_eval/src/interpret/cast.rs:444:25:
assertion failed: &vtable_entries[..vtable_entries_b.len()] == vtable_entries_b

Same for the test in #135316.

Maybe we should just always enable these checks for Miri? That makes virtual calls slightly more expensive, but it seems hard to imagine that becoming a bottleneck.

@compiler-errors
Copy link
Member Author

Maybe we should just always enable these checks for Miri?

Yeah, my first reaction was to test it in miri and not it triggering did give me a false impression that miri was not affected here :) I'll change the debug assertion to be a regular assertion.

@RalfJung
Copy link
Member

Miri is indeed not affected, but it'd still be nice to have Miri catch issues that only affect code using the actual vtable entries.

@rustbot
Copy link
Collaborator

rustbot commented Jan 11, 2025

The Miri subtree was changed

cc @rust-lang/miri

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2025
Fix deduplication mismatches in vtables leading to upcasting unsoundness

We currently have two cases where subtleties in supertraits can trigger disagreements in the vtable layout, e.g. leading to a different vtable layout being accessed at a callsite compared to what was prepared during unsizing. Namely:

### rust-lang#135315

In this example, we were not normalizing supertraits when preparing vtables. In the example,

```
trait Supertrait<T> {
    fn _print_numbers(&self, mem: &[usize; 100]) {
        println!("{mem:?}");
    }
}
impl<T> Supertrait<T> for () {}

trait Identity {
    type Selff;
}
impl<Selff> Identity for Selff {
    type Selff = Selff;
}

trait Middle<T>: Supertrait<()> + Supertrait<T> {
    fn say_hello(&self, _: &usize) {
        println!("Hello!");
    }
}
impl<T> Middle<T> for () {}

trait Trait: Middle<<() as Identity>::Selff> {}
impl Trait for () {}

fn main() {
    (&() as &dyn Trait as &dyn Middle<()>).say_hello(&0);
}
```

When we prepare `dyn Trait`, we see a supertrait of `Middle<<() as Identity>::Selff>`, which itself has two supertraits `Supertrait<()>` and `Supertrait<<() as Identity>::Selff>`. These two supertraits are identical, but they are not duplicated because we were using structural equality and *not* considering normalization. This leads to a vtable layout with two trait pointers.

When we upcast to `dyn Middle<()>`, those two supertraits are now the same, leading to a vtable layout with only one trait pointer. This leads to an offset error, and we call the wrong method.

### rust-lang#135316

This one is a bit more interesting, and is the bulk of the changes in this PR. It's a bit similar, except it uses binder equality instead of normalization to make the compiler get confused about two vtable layouts. In the example,

```
trait Supertrait<T> {
    fn _print_numbers(&self, mem: &[usize; 100]) {
        println!("{mem:?}");
    }
}
impl<T> Supertrait<T> for () {}

trait Trait<T, U>: Supertrait<T> + Supertrait<U> {
    fn say_hello(&self, _: &usize) {
        println!("Hello!");
    }
}
impl<T, U> Trait<T, U> for () {}

fn main() {
    (&() as &'static dyn for<'a> Trait<&'static (), &'a ()>
        as &'static dyn Trait<&'static (), &'static ()>)
        .say_hello(&0);
}
```

When we prepare the vtable for `dyn for<'a> Trait<&'static (), &'a ()>`, we currently consider the PolyTraitRef of the vtable as the key for a supertrait. This leads two two supertraits -- `Supertrait<&'static ()>` and `for<'a> Supertrait<&'a ()>`.

However, we can upcast[^up] without offsetting the vtable from `dyn for<'a> Trait<&'static (), &'a ()>` to `dyn Trait<&'static (), &'static ()>`. This is just instantiating the principal trait ref for a specific `'a = 'static`. However, when considering those supertraits, we now have only one distinct supertrait -- `Supertrait<&'static ()>` (which is deduplicated since there are two supertraits with the same substitutions). This leads to similar offsetting issues, leading to the wrong method being called.

[^up]: I say upcast but this is a cast that is allowed on stable, since it's not changing the vtable at all, just instantiating the binder of the principal trait ref for some lifetime.

The solution here is to recognize that a vtable isn't really meaningfully higher ranked, and to just treat a vtable as corresponding to a `TraitRef` so we can do this deduplication more faithfully. That is to say, the vtable for `dyn for<'a> Tr<'a>` and `dyn Tr<'x>` are always identical, since they both would correspond to a set of free regions on an impl... Do note that `Tr<for<'a> fn(&'a ())>` and `Tr<fn(&'static ())>` are still distinct.

----

There's a bit more that can be cleaned up. In codegen, we can stop using `PolyExistentialTraitRef` basically everywhere. We can also fix SMIR to stop storing `PolyExistentialTraitRef` in its vtable allocations.

As for testing, it's difficult to actually turn this into something that can be tested with `rustc_dump_vtable`, since having multiple supertraits that are identical is a recipe for ambiguity errors. Maybe someone else is more creative with getting that attr to work, since the tests I added being run-pass tests is a bit unsatisfying. Miri also doesn't help here, since it doesn't really generate vtables that are offset by an index in the same way as codegen.

r? `@lcnr` for the vibe check? Or reassign, idk. Maybe let's talk about whether this makes sense.

<sup>(I guess an alternative would also be to not do any deduplication of vtable supertraits (or only a really conservative subset) rather than trying to normalize and deduplicate more faithfully here. Not sure if that works and is sufficient tho.)</sup>

cc `@steffahn` -- ty for the minimizations
cc `@WaffleLapkin` -- since you're overseeing the feature stabilization :3

Fixes rust-lang#135315
Fixes rust-lang#135316
@bors
Copy link
Contributor

bors commented Jan 29, 2025

⌛ Testing commit 526d71e with merge 5d8da00...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 29, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 29, 2025
@lcnr
Copy link
Contributor

lcnr commented Jan 29, 2025

That looks like it may be a proper error and requires a higher recursion limit/manual auto trait impl in fuchsia?

@bors
Copy link
Contributor

bors commented Jan 30, 2025

☔ The latest upstream changes (presumably #136035) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Jan 30, 2025

📌 Commit d98b99a has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2025
@izagawd
Copy link

izagawd commented Jan 30, 2025

Thank you for your hard work, compiler-errors.

@bors
Copy link
Contributor

bors commented Jan 31, 2025

⌛ Testing commit d98b99a with merge c37fbd8...

@bors
Copy link
Contributor

bors commented Jan 31, 2025

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c37fbd8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 31, 2025
@bors bors merged commit c37fbd8 into rust-lang:master Jan 31, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 31, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
Ensure that we never try to monomorphize the upcasting of impossible dyn types

Just the last commit, blocked on rust-lang#135318.

r? lcnr
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c37fbd8): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.4% [0.4%, 0.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -4.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.3% [-4.3%, -4.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -4.3% [-4.3%, -4.3%] 1

Cycles

Results (secondary -0.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.7%, -0.7%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 777.277s -> 776.084s (-0.15%)
Artifact size: 328.97 MiB -> 328.86 MiB (-0.03%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet